-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(c-api) Add wat2wasm
to the wasm-c-api
module
#1635
Conversation
It depends on wasmerio/wasmer#1635. The `include/` and `wasmer_wasm.h` files are already from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed organization of the C APIs privately, and I think with some of the changes we talked about we can probably generate that file, but for now it's non-trivial to do so.
I'm unsure if we want to make this transformation explicit or not. In Wasmer we allow taking a &[u8]
and we'll handle both wat and wasm. Perhaps we just want to do that for the C API too.
If we want to make it explicit, then this change seems fine. How would users free this data though? And one more point, when turning things like Vec
or String
into a pointer + length, it's important to shrink_to_fit
first so we don't leak memory. Vec
has 3 numbers: pointer, length, and capacity. If capacity != length (which is very likely), then we can't reconstruct the Vec
correctly and will probably do a partial free
c1a9c8b
to
ded3132
Compare
I've rebased this PR on top of the - unsafe extern "C" fn wat2wasm(wat: &wasm_byte_vec_t, wasm: &mut wasm_byte_vec_t) -> Option<()>
+ unsafe extern "C" fn wat2wasm(wat: &wasm_byte_vec_t) -> Option<Box<wasm_byte_vec_t>> I believe this API is better. If it returns Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Other than some small bugs it looks good to go! One other thing, perhaps we want to add an example/test for this function (it may have caught the use after free bug, though it's highly likely it would not have)
bors r+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Build succeeded: |
This PR adds the
wat2wasm
function to thewasm-c-api
module.Caveat of this patch: The
wasmer_wasm.h
seems to be written by hand. Consequently, if thelib/c-api/
crate is compiled without thewat
feature (which is the default), thewat2wasm
function will still be defined inwasmer_wasm.h
.Proposal: We must generate the
wasmer_wasm.h
header file with the same approach than we use forwasm.h
. It can be done in a next PR.Thoughts @MarkMcCaskey